Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: --spec now allows () in filename #29279

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

TomasTeixeira2003
Copy link

Additional details

Even though there was an existing workaround, it is easier to just allow parenthesis.

PR Tasks

@cypress-app-bot
Copy link
Collaborator

@jennifer-shehane jennifer-shehane self-requested a review April 9, 2024 17:40
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@TomasTeixeira2003
Copy link
Author

TomasTeixeira2003 commented Apr 9, 2024

Is there anything I should do since the check for ''Semantic Pull Request'' is failing?

cli/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this manually and confirmed that the tests now run when specifying a spec with opening and closing parens.

Before

Screenshot 2024-04-11 at 1 53 41 PM

After

Screenshot 2024-04-11 at 1 50 03 PM

@AtofStryker AtofStryker self-requested a review April 12, 2024 13:24
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we want to escape parenthesis in all cases in cypress. the --spec arg can support glob patterns and parenthesis are a valid glob pattern, but I haven't been able to get glob patterns outside of wildcards to work correctly. @jennifer-shehane do you have more knowledge on what's expected? I mainly going off of https://docs.cypress.io/guides/guides/command-line#cypress-run-spec-lt-spec-gt and testing with the CLI and using https://globster.xyz/

cli/CHANGELOG.md Outdated Show resolved Hide resolved
@AtofStryker AtofStryker changed the title fix: --spec now allows () in filename (https://github.com/cypress-io/cypress/issues/28509) fix: --spec now allows () in filename Apr 12, 2024
jennifer-shehane and others added 2 commits April 12, 2024 17:07
Co-authored-by: Bill Glesias <bglesias@gmail.com>
@jennifer-shehane
Copy link
Member

jennifer-shehane commented Apr 12, 2024

@AtofStryker That's a good call out. This bug only happens when you have a closing paren DIRECTLY following an opening paren, so () not (a). I don't think that would be a valid usecase in any globs 🤔 , so escaping them back to back should be fine. That's what I'm thinking.

@AtofStryker
Copy link
Contributor

@jennifer-shehane is the failure in CI legit in relation to the glob changes?

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasTeixeira2003 There does appear to be a snapshot that has changed with globs - that is failing here. Can you take a look: https://app.circleci.com/pipelines/github/cypress-io/cypress/60955/workflows/e499d77f-ccf3-4aba-8f7f-ed00d2595ba1/jobs/2535654

Screenshot 2024-04-16 at 2 19 19 PM

@TomasTeixeira2003
Copy link
Author

I will look into it today

@jennifer-shehane
Copy link
Member

@tomasbjerre Any chance to look at this?

@tomasbjerre
Copy link
Contributor

@tomasbjerre Any chance to look at this?

@jennifer-shehane you probably tagged the wrong Tomas. But I think it looks great :)

@jennifer-shehane
Copy link
Member

Sorry @TomasTeixeira2003 Any chance to look at this?

@TomasTeixeira2003
Copy link
Author

I'm sorry, have had a lot of work lately. By tomorrow I will check it.

@jennifer-shehane
Copy link
Member

@TomasTeixeira2003 Just pinging to check in on these updates again.

@jennifer-shehane
Copy link
Member

@TomasTeixeira2003 Any chance you'll be able to fix this PR? We'll need to close otherwise.

@TomasTeixeira2003
Copy link
Author

I've been working on another feature to rerun failed tests and didn't remember about this. Sorry. By Sunday i will have it working.

@TomasTeixeira2003
Copy link
Author

What is the yarn command to run only the system-tests?

@jennifer-shehane
Copy link
Member

@jennifer-shehane
Copy link
Member

Rerunning the tests here

@jennifer-shehane
Copy link
Member

@TomasTeixeira2003 There are some linting errors

@TomasTeixeira2003
Copy link
Author

And the other ones are flaky?

@jennifer-shehane jennifer-shehane self-requested a review June 21, 2024 18:52
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasTeixeira2003 I'm not seeing this fix working anymore when passing the --spec arg. I understand this may be isolated to running Cypress in the local mode, but I can't see the change fixing the issue now with the recent changes. Including a log of what I was running. The test.cy.js file ran, but the test().cy.js file did not run.

Screenshot 2024-06-24 at 9 53 01 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cypress doesn't like parenthesis () in filename
5 participants